Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

schema/gen/go: avoid Maybe pointers for small types #160

Merged
merged 1 commit into from
Apr 7, 2021
Merged

schema/gen/go: avoid Maybe pointers for small types #160

merged 1 commit into from
Apr 7, 2021

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Apr 6, 2021

(see commit message)

@mvdan mvdan requested a review from warpfork April 6, 2021 14:23
If we know that a schema type can be represented in Go with a small
amount of bytes, using a pointer to store its "maybe" is rarely a good
idea. For example, an optional string only weighs twice as much as a
pointer, so a pointer adds overhead and will barely ever save any
memory.

Add a function to work out the byte size of a schema.TypeKind, relying
on reflection and the basicnode package. Debug prints are also present
if one wants to double-check the numbers. As of today, they are:

	sizeOf(small): 32 (4x pointer size)
	sizeOf(Bool): 1
	sizeOf(Int): 8
	sizeOf(Float): 8
	sizeOf(String): 16
	sizeOf(Bytes): 24
	sizeOf(List): 24
	sizeOf(Map): 32
	sizeOf(Link): 16

Below is the result on go-merkledag's BenchmarkRoundtrip after
re-generating go-codec-dagpb with this change. Note that the dag-pb
schema contains multiple optional fields, such as strings.

	name         old time/op    new time/op    delta
	Roundtrip-8    4.24µs ± 3%    3.78µs ± 0%  -10.87%  (p=0.004 n=6+5)

	name         old alloc/op   new alloc/op   delta
	Roundtrip-8    6.38kB ± 0%    6.24kB ± 0%   -2.26%  (p=0.002 n=6+6)

	name         old allocs/op  new allocs/op  delta
	Roundtrip-8       103 ± 0%        61 ± 0%  -40.78%  (p=0.002 n=6+6)

Schema typekinds which don't directly map to basicnode prototypes, such
as structs and unions, are left as a TODO for now.

I did not do any measurements to arrive at the magic number of 4x, which
is documented in the code. We might well increase it in the future, with
more careful benchmarking. For now, it seems like a conservative starting
point that should cover all basic types.

Finally, re-generate within this repo.
@mvdan
Copy link
Contributor Author

mvdan commented Apr 7, 2021

I've re-done this with reflection and basicnode, which means it's no longer an estimation, and will stay up to date as the internals change.

If there is a better place to grab Go values/types than basicnode for these measurements, I'm all ears. Perhaps some code already generated by schema/gen/go?

Anyway, there's zero behavioral change compared to before. Another go generate ./... run produced zero changes in both repos, so the benchmark numbers are still up to date.

@warpfork
Copy link
Collaborator

warpfork commented Apr 7, 2021

Ahh, right. We can't readily sizeof on the types before they're generated. This does still seem like a reasonable heuristic though, and certainly does do the right thing right now for practical values. 👍

@warpfork warpfork self-requested a review April 7, 2021 12:30
Copy link
Collaborator

@warpfork warpfork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge freely :)

@mvdan mvdan merged commit f3d42e0 into ipld:master Apr 7, 2021
@mvdan mvdan deleted the gen-pointers-small branch April 7, 2021 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants